Fix CI coverage-comment failure on push to main - #100
Merged
Conversation
exception-table.ts declared ExceptionTableEntry.catchType as `any | null`, which eslint flags as a redundant union and which also masked a latent type mismatch (the field actually holds a ConstantClass, not a ClassData, until resolved). Narrow it to ConstantClass | null to match how it's populated in Attributes.ts and consumed in thread.ts. The pre-commit hook's repo-wide `eslint --fix` and `prettier --write` also picked up and applied a handful of pre-existing import-order and formatting fixes in other files that were tripping the hook. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TujL5U9Kg6vHKvRR7zryE4
jest-coverage-report-action tries to post a PR comment even on direct pushes to main, where no PR exists, causing the build job to fail with "Resource not accessible by integration". Only run the coverage-comment action on pull_request events, and run plain yarn test on push. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TujL5U9Kg6vHKvRR7zryE4
Coverage report
Test suite run success1148 tests passing in 65 suites. Report generated by 🧪jest coverage report action from 1381f6c |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The workflow change directly addresses the documented CI failure mode and the remaining edits are low-risk type/format adjustments, with only a small lint/style nit noted in review comments.
Pull request overview
This PR prevents the CI build job from failing on push to main by ensuring the Jest coverage action only attempts to post PR comments on pull_request events, while still running tests on push. It also tightens JVM exception-table typing and applies a set of small formatting/lint cleanups in compiler/JVM code.
Changes:
- Gate
ArtiomTr/jest-coverage-report-action@v2topull_requestevents and runyarn testdirectly on non-PR events. - Narrow JVM
ExceptionTableEntry.catchTypetoConstantClass | null(instead ofany | null) and adjust related signatures/imports. - Misc formatting/import tidy-ups across compiler and code generator modules.
File summaries
| File | Description |
|---|---|
| src/types/typeFactories/methodFactory.ts | Minor cleanup in throws-clause handling (member-access style). |
| src/jvm/types/class/Method.ts | Import ordering adjustment related to exception-table usage. |
| src/jvm/exception-table.ts | Narrow catchType typing to ConstantClass | null and update insert signature. |
| src/compiler/symbol-table.ts | Import ordering + formatting in final-method override check. |
| src/compiler/error.ts | Formatting cleanup. |
| src/compiler/compiler.ts | Formatting cleanup for class symbol insertion / output array. |
| src/compiler/code-generator.ts | Import ordering and formatting cleanup in code generation paths. |
| src/compiler/binary-writer.ts | Formatting cleanup for normalizeClassFile signature. |
| .github/workflows/build.yml | Gate coverage comment step to PRs; add explicit test step for push-to-main runs. |
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
kjw142857
self-requested a review
September 2, 2026 02:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ArtiomTr/jest-coverage-report-action@v2's defaultoutput: commentstep tries to post a PR comment even when the workflow was triggered by a directpushtomain(e.g. after merging a PR), where there's no associated PR — this fails withHttpError: Resource not accessible by integrationand marks the whole build job as failed even though build/tests pass. See failing run: https://github.com/source-academy/java-slang/actions/runs/33060105554/job/98476472987pull_requestevents only; run plainyarn testonpush.ExceptionTableEntry.catchTypewas typedany | null(redundant union, eslint error), which also masked that the field actually holds aConstantClass(not aClassData) until resolved via.resolve(). Narrowed toConstantClass | nullto match how it's populated inAttributes.tsand consumed inthread.ts.Test plan
yarn tsc --noEmitpassesyarn testpasses (65 suites / 1148 tests)buildworkflow succeeds on both apull_requestrun (coverage comment posts) and apush-to-mainrun (no failure) after merge🤖 Generated with Claude Code
https://claude.ai/code/session_01TujL5U9Kg6vHKvRR7zryE4